Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spinachboul update function #457

Closed
wants to merge 12 commits into from
Closed

Spinachboul update function #457

wants to merge 12 commits into from

Conversation

Spinachboul
Copy link
Contributor

Changes Made

  • Explained Tensor Product Functions for the user's reference.
  • Changed the function into a more readable form.
  • Attached the relevant plots for different Surrogate Models for visualization.

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aa744e5) 78.12% compared to head (688fee1) 78.12%.
Report is 13 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #457   +/-   ##
=======================================
  Coverage   78.12%   78.12%           
=======================================
  Files          23       23           
  Lines        3154     3155    +1     
=======================================
+ Hits         2464     2465    +1     
  Misses        690      690           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Spinachboul Spinachboul mentioned this pull request Dec 29, 2023
5 tasks
@Spinachboul
Copy link
Contributor Author

@ChrisRackauckas and @sathvikbhagavan
Please have a look at the changes made in the readme file and suggest anything which could be improved!!

Comment on lines 47 to 55
![kriging](https://github.com/Spinachboul/Surrogates.jl/assets/105979087/906e6688-db47-48be-90d1-ea471aacac16)

## Lobachevsky Plot

![lobachevsky](https://github.com/Spinachboul/Surrogates.jl/assets/105979087/678cfc13-0aec-4488-8e4d-39649853ecdd)

## Combined Plot

![combined_plot](https://github.com/Spinachboul/Surrogates.jl/assets/105979087/46762f0d-50c5-4d6c-961a-236fd9fb3ad5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, but the docs already generate these plots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's right! I attached the plots for reference about Kriging and Lobachevsky individually, and their plotting curves. And can't we attach plots both, in the readme and the docs??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't recommend attaching plots but instead running code to generate them

Comment on lines 1 to 15
# GEKPLS Function

Gradient Enhanced Kriging with Partial Least Squares Method (GEKPLS) is a surrogate modelling technique that brings down computation time and returns improved accuracy for high-dimensional problems. The Julia implementation of GEKPLS is adapted from the Python version by [SMT](https://github.com/SMTorg) which is based on this [paper](https://arxiv.org/pdf/1708.02663.pdf).

# Modifications for Improved GEKPLS Function:

To enhance the GEKPLS function, sampling method was changed from ```SobolSample()``` to ```HaltonSample()```.


```@example gekpls_water_flow

using Surrogates
using Zygote

function water_flow(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just GEKPLS, why would it have a different page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I added that in the Pages.jl for the modifications I made in the new file, I will remove it!

Comment on lines 52 to 55
| **Sampling Method** | **RMSE** | **Differences** |
|----------------------|--------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| **Sobol Sampling** | 0.021472963465423097 | Utilizes digital nets to generate quasi-random numbers, offering low discrepancy points for improved coverage. - Requires careful handling, especially in higher dimensions. |
| **Halton Sampling** | 0.02144270998045834 | Uses a deterministic sequence based on prime numbers to generate points, allowing for quasi-random, low-discrepancy sampling. - Simpler to implement but may exhibit correlations in some dimensions affecting coverage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more for QuasiMonteCarlo.jl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like we talked about in #456, GEKPLS is independent from sampling method and the notion of improvement in accuracy depends on the original problem itself and what sampling works better. There is no general recipe for making it better. So, I don't think this tutorial is adding any new value and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sathvikbhagavan
Your'e right! If anything, we could have better optimization techniques to improve in gekpls.jl, instead of solely focusing on sampling methods! So, I have removed the file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you have removed it. You might have forgot to push it. Look at the diff and commits to confirm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete Improvedgekpls.md and then we can merge this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I have deleted the file Improvedgekpls.md
Please check once again if all the changes are suitable for merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisRackauckas and @sathvikbhagavan
Is this PR ready for merging? Or still there are things which need to be modified??


## Kriging Plot

![kriging](https://github.com/Spinachboul/Surrogates.jl/assets/105979087/906e6688-db47-48be-90d1-ea471aacac16)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary. The plots would be generated from the above code block. Are you doing anything new, as in what code produces this plot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do anything new logically in the code, just added in some explanation about the tensor product function which I thought would be relevant and added the plots , i will remove if it is not required!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sathvikbhagavan
But then, shouldn't we add more mathematical explanation to improve our readme file, simply stating the function would be too vague for the viewers!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the usefulness of the plots. They don't provide any new information either. Look at the current plot in the tutorial - https://docs.sciml.ai/Surrogates/stable/tensor_prod/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so for now, I just need to focus on adding mathematical explanations in the tutorials right?
So soon I'll share a sample file (won't hit a PR for this) and then request your approval if it is going along the correct lines.
Please then suggest to me the correct blend of theory and math explanations which would enable me to work efficiently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Feel free to open up a PR and continue working. Make it a draft PR and you can push in commits. When you are ready, make it ready for review and ping me. I will take a look. Hopefully you are more comfortable in the ecosystem and how things work in SciML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! I am actually getting to know SciML codebase better. Thanks for your guidance throughout this time.

Copy link
Contributor Author

@Spinachboul Spinachboul Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ChrisRackauckas and @sathvikbhagavan
Please let me know if this explanation suits well with the Kriging Model!

pic2

pic1

I was wondering if this is how we could expand the docs in the tutorials section!!
(Please forgive for the bad handwriting!! 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please also suggest models on which I could work! I mean there are many out there but the ones which are important and really catch reader's eye.

docs/src/tensor_prod.md Outdated Show resolved Hide resolved

For instance, consider a tensor product function defined as follows:

```\[ f(x) = ∏ᵢ=₁ᵈ cos(aπxᵢ) \]```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from what it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is the same just in a more readable form than before, the limits and the function, which we usually write!
As earlier it was a bit confusing as to what the actual function is!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, then its fine

@@ -1,6 +1,5 @@
# Tensor product function
The tensor product function is defined as:
``f(x) = \prod_{i=1}^d \cos(a\pi x_i)``
The tensor product function is defined as: ```\[ f(x) = ∏ᵢ=₁ᵈ cos(aπxᵢ) \]```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this isn't correct. This is one example of a tensor product.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is true. Maybe we specifically say it is one of the possible forms. I think this was taken from https://smt.readthedocs.io/en/latest/_src_docs/problems/tensorproduct.html

Copy link
Contributor Author

@Spinachboul Spinachboul Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, @ChrisRackauckas and @sathvikbhagavan
Should we properly define the tensor product function like this:
equation

Or just stating this example would be fine??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would do it. But do we have an actual issue that is being solved here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the benchmarks need some cleanup, I think we can startoff with this PR to clean up the tensor product benchmark? Although I am not sure why would we need a surrogate for computing tensor product 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sathvikbhagavan
I think there would be a couple of factors as to why surrogates could be used to compute tensor product:

  • Computational Complexity would be decreased
  • Efficient memory handling
  • Normalization of products of tensors which provide numerical stability

This PR #457 only consists of changing the example of tensor product into a more readable form up till now, but I would also work towards explaining the mathematical concepts behind surrogate models in my draft PR as we discussed earlier.

@Spinachboul Spinachboul closed this by deleting the head repository Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants